Refactor setups_tag tests to use direct method calls for non-happy-path tests#298
Conversation
WalkthroughTests in tests/routers/openml/setups_tag_test.py were refactored to call routers.openml.setups.tag_setup directly for most scenarios instead of exercising the /setup/tag HTTP endpoint. The "unknown setup" test now asserts that SetupNotFoundError is raised with a matching message; the "already exists" test pre-inserts a conflicting setup_tag row and asserts TagAlreadyExistsError with a tag-specific message; the unit "success" test validates the returned payload contains the inserted tag and verifies persistence via a parameterized SQL query. An HTTP-based success test marked with Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the success tests you assert
"my_*_tag" in ...["tag"]; if the tag is expected to match exactly, consider changing these to strict equality to catch unexpected formatting or concatenation issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the success tests you assert `"my_*_tag" in ...["tag"]`; if the tag is expected to match exactly, consider changing these to strict equality to catch unexpected formatting or concatenation issues.
## Individual Comments
### Comment 1
<location path="tests/routers/openml/setups_tag_test.py" line_range="29-30" />
<code_context>
- assert re.match(r"Setup \d+ not found.", response.json()["detail"])
+
+ assert response.status_code == HTTPStatus.OK
+ assert "my_new_success_api_tag" in response.json()["setup_tag"]["tag"]
+
+ rows = await expdb_test.execute(
</code_context>
<issue_to_address>
**suggestion (testing):** Use stricter equality assertions for tags instead of substring checks.
Both the API success test and the direct success test use `in` to check the tag value. If the API doesn’t intentionally add extra content around the tag, please assert the exact value (or full structure) instead, e.g.:
```python
assert response.json()["setup_tag"]["tag"] == "my_new_success_api_tag"
```
This makes the tests stricter and prevents false positives if additional text is ever added to the tag.
```suggestion
assert response.status_code == HTTPStatus.OK
assert response.json()["setup_tag"]["tag"] == "my_new_success_api_tag"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #298 +/- ##
==========================================
- Coverage 54.20% 52.43% -1.77%
==========================================
Files 38 38
Lines 1581 1661 +80
Branches 137 154 +17
==========================================
+ Hits 857 871 +14
- Misses 722 788 +66
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/routers/openml/setups_tag_test.py (2)
42-42: Tighten exception regex to avoid permissive matches.Both patterns currently end with
.(any char in regex) and are unanchored, so they can pass on unintended messages. Use anchored patterns and escape the final period.Proposed assertion pattern fix
- with pytest.raises(SetupNotFoundError, match=r"Setup \d+ not found."): + with pytest.raises(SetupNotFoundError, match=r"^Setup \d+ not found\.$"): ... - with pytest.raises(TagAlreadyExistsError, match=r"Setup 1 already has tag 'existing_tag_123'."): + with pytest.raises( + TagAlreadyExistsError, + match=r"^Setup 1 already has tag 'existing_tag_123'\.$", + ):Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_tag_test.py` at line 42, The regex in the pytest.raises calls for SetupNotFoundError is too permissive — update the match patterns used in the pytest.raises(SetupNotFoundError, match=...) assertions to anchor the start and end and escape the final period (e.g., change to a pattern equivalent to "^Setup \\d+ not found\\.$"); apply the same anchored/escaped fix to the other similar assertion referenced (the second pytest.raises at the other location) so the tests only match the exact expected messages.
32-35: Prefer bound SQL parameters in DB assertions.Inline SQL string literals are brittle for tags with quotes and harder to reuse; parameterized
text(...)keeps the assertions safer and clearer.Proposed query refactor
- rows = await expdb_test.execute( - text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'my_new_success_api_tag'") - ) + rows = await expdb_test.execute( + text("SELECT * FROM setup_tag WHERE id = :setup_id AND tag = :tag"), + {"setup_id": 1, "tag": "my_new_success_api_tag"}, + ) ... - rows = await expdb_test.execute( - text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'my_direct_success_tag'") - ) + rows = await expdb_test.execute( + text("SELECT * FROM setup_tag WHERE id = :setup_id AND tag = :tag"), + {"setup_id": 1, "tag": "my_direct_success_tag"}, + )Also applies to: 75-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_tag_test.py` around lines 32 - 35, Replace the inline SQL literals in the test DB assertions with parameterized SQL using text(...) and bound parameters: update the expdb_test.execute calls that query the setup_tag table (currently using "id = 1 AND tag = 'my_new_success_api_tag'") to use named bind params (e.g., :id, :tag) and pass the values in the execute call (e.g., {"id": 1, "tag": "my_new_success_api_tag"}); make the same change for the other assertion that queries setup_tag (the second expdb_test.execute around the later assertion) so both queries use bound parameters instead of inline string literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/routers/openml/setups_tag_test.py`:
- Line 42: The regex in the pytest.raises calls for SetupNotFoundError is too
permissive — update the match patterns used in the
pytest.raises(SetupNotFoundError, match=...) assertions to anchor the start and
end and escape the final period (e.g., change to a pattern equivalent to "^Setup
\\d+ not found\\.$"); apply the same anchored/escaped fix to the other similar
assertion referenced (the second pytest.raises at the other location) so the
tests only match the exact expected messages.
- Around line 32-35: Replace the inline SQL literals in the test DB assertions
with parameterized SQL using text(...) and bound parameters: update the
expdb_test.execute calls that query the setup_tag table (currently using "id = 1
AND tag = 'my_new_success_api_tag'") to use named bind params (e.g., :id, :tag)
and pass the values in the execute call (e.g., {"id": 1, "tag":
"my_new_success_api_tag"}); make the same change for the other assertion that
queries setup_tag (the second expdb_test.execute around the later assertion) so
both queries use bound parameters instead of inline string literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd2301c9-25fe-43e9-abd7-cc6ab9b01255
📒 Files selected for processing (1)
tests/routers/openml/setups_tag_test.py
|
@PGijsbers This PR refactors setups_tag tests per the post-#295 structure (single HTTP happy path + direct handler calls). I plan to open additional small PRs for other endpoints the same way unless you’d prefer a different split. Thanks |
|
One per endpoint is great! That would make sure the PRs are contained to individual files as well with the new structure. I made a separate issue for the tests on the newly added list tasks endpoint that will be picked up by the author of that function (#300). I did not make dedicated issues for the other files yet, but you can go ahead anyway. |
There was a problem hiding this comment.
I made a suggestion to validate the entire JSON, but I'll just commit it directly. Otherwise looks good! I think there is some other opportunity to improve the tests but that can be addressed separately (in part because the tagging logic is likely to be extracted centrally and can be tested just once).
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/routers/openml/setups_tag_test.py (1)
31-31: Prefer the localtagvariable in the expected payload.This avoids literal drift if the test input changes.
Proposed refactor
- expected = {"setup_tag": {"id": "1", "tag": ["setup_tag_via_http"]}} + expected = {"setup_tag": {"id": "1", "tag": [tag]}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_tag_test.py` at line 31, The test currently hardcodes the expected payload value for "tag" instead of reusing the local tag variable; update the expected dict (the variable named expected) to reference the existing tag variable for the "setup_tag" -> "tag" value so the assertion uses the same tag source as the request (locate the expected assignment in tests/routers/openml/setups_tag_test.py and replace the literal ["setup_tag_via_http"] with the tag variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/routers/openml/setups_tag_test.py`:
- Line 45: The pytest assertion using pytest.raises for SetupNotFoundError
currently uses the regex r"Setup \d+ not found." where the trailing dot is
unescaped; update the match to either escape the period (r"Setup \d+ not
found\.") or assert the exact message (e.g., match=r"^Setup \d+ not found\.$")
to prevent the dot acting as a wildcard and ensure the error message is matched
precisely in the with pytest.raises(...) line.
---
Nitpick comments:
In `@tests/routers/openml/setups_tag_test.py`:
- Line 31: The test currently hardcodes the expected payload value for "tag"
instead of reusing the local tag variable; update the expected dict (the
variable named expected) to reference the existing tag variable for the
"setup_tag" -> "tag" value so the assertion uses the same tag source as the
request (locate the expected assignment in
tests/routers/openml/setups_tag_test.py and replace the literal
["setup_tag_via_http"] with the tag variable).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2a21dcf-ce46-4d61-93b6-0a70e95a0174
📒 Files selected for processing (1)
tests/routers/openml/setups_tag_test.py
|
|
||
|
|
||
| async def test_setup_tag_unknown_setup(expdb_test: AsyncConnection) -> None: | ||
| with pytest.raises(SetupNotFoundError, match=r"Setup \d+ not found."): |
There was a problem hiding this comment.
Tighten the exception regex in Line 45.
not found. uses . as a wildcard. Escape the trailing period (or assert the exact message) to avoid false positives.
Proposed fix
- with pytest.raises(SetupNotFoundError, match=r"Setup \d+ not found."):
+ with pytest.raises(SetupNotFoundError, match=r"Setup \d+ not found\."):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(SetupNotFoundError, match=r"Setup \d+ not found."): | |
| with pytest.raises(SetupNotFoundError, match=r"Setup \d+ not found\."): |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/routers/openml/setups_tag_test.py` at line 45, The pytest assertion
using pytest.raises for SetupNotFoundError currently uses the regex r"Setup \d+
not found." where the trailing dot is unescaped; update the match to either
escape the period (r"Setup \d+ not found\.") or assert the exact message (e.g.,
match=r"^Setup \d+ not found\.$") to prevent the dot acting as a wildcard and
ensure the error message is matched precisely in the with pytest.raises(...)
line.
Got it, thanks! I’ll proceed with one PR per endpoint. |
2 participants
Summary:
Following the testing guidelines from #295:
py_api.